Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Verify supported Elasticsearch distribution during license reconciliation #4920

Merged
merged 10 commits into from
Oct 6, 2021
Merged

Verify supported Elasticsearch distribution during license reconciliation #4920

merged 10 commits into from
Oct 6, 2021

Conversation

thbkrkr
Copy link
Contributor

@thbkrkr thbkrkr commented Oct 1, 2021 •
edited
Loading

This commit makes ECK more strict against ES license reconciliation errors to verify that it is managing a supported Elasticsearch distribution.

This produces events:

Warning Unexpected 7s (x14 over 54s) elasticsearch-controller Unsupported Elasticsearch distribution: unable to verify Elasticsearch license: 400 Bad Request: Invalid index name [license], must not start with ''.

And logs:

2021-10-04T16:17:58.683+0200    ERROR   manager.eck-operator.controller.elasticsearch-controller        Reconciler error        {
    "service.version": "1.9.0-SNAPSHOT+ec9237d8",
    "name": "os",
    "namespace": "default",
    "error": "unsupported elasticsearch distribution: unable to verify Elasticsearch license: 400 Bad Request: unknown",
    "errorCauses": [
        {
            "error": "unsupported elasticsearch distribution: unable to verify Elasticsearch license: 400 Bad Request: unknown",
            "errorVerbose": "400 Bad Request: unknown
            unable to verify Elasticsearch license
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/license.checkElasticsearchLicense
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/license.Reconcile
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/driver.(*defaultDriver).Reconcile
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch.(*ReconcileElasticsearch).internalReconcile
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch.(*ReconcileElasticsearch).Reconcile
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
            runtime.goexit
            unsupported elasticsearch distribution
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/driver.(*defaultDriver).Reconcile
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch.(*ReconcileElasticsearch).internalReconcile
            github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch.(*ReconcileElasticsearch).Reconcile
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
            sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
            runtime.goexit
        }
    ]
}

@thbkrkr thbkrkr added the >enhancement Enhancement of existing functionality label Oct 1, 2021
@thbkrkr thbkrkr force-pushed the stop-reconciliation-on-es-license-reconciliation-error branch from a6f654c to 2fc7825 Compare October 4, 2021 10:50
pebrc
pebrc reviewed Oct 4, 2021
View reviewed changes
pkg/controller/elasticsearch/license/reconcile.go Outdated
currentLicense, err := clusterClient.GetLicense(ctx)
if err != nil {
// 4xx is not supported, except 404 which may happen if the master node is generating a new cluster state
unsupportedElasticsearch := esclient.Is4xx(err) && !esclient.IsNotFound(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle 401/403 separately? In case a user misconfigured auth realms and locks us out? We could have a separate warning saying something like: "Unable to verify Elasticsearch license due to lack of privileges, check your security configuration"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's make sure we don't lock the cluster down for these errors.

pkg/controller/elasticsearch/license/reconcile.go Show resolved Hide resolved
pkg/controller/elasticsearch/driver/driver.go Outdated Show resolved Hide resolved
@thbkrkr thbkrkr force-pushed the stop-reconciliation-on-es-license-reconciliation-error branch 2 times, most recently from c66ee57 to 88cdf5e Compare October 4, 2021 14:03
@thbkrkr thbkrkr force-pushed the stop-reconciliation-on-es-license-reconciliation-error branch from 88cdf5e to ec9237d Compare October 4, 2021 14:04
@thbkrkr
Copy link
Contributor Author

thbkrkr commented Oct 4, 2021

Jenkins test this please

@thbkrkr thbkrkr changed the title Stop reconciliation on ES license reconciliation error Verify supported Elasticsearch distribution during licence reconciliation Oct 5, 2021
@thbkrkr thbkrkr added the v1.9.0 label Oct 5, 2021
pebrc
pebrc reviewed Oct 6, 2021
View reviewed changes
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few nits but otherwise LGTM 👍

pkg/controller/elasticsearch/license/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/driver/driver.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/driver/driver.go Outdated Show resolved Hide resolved
@thbkrkr thbkrkr merged commit 53865df into elastic:master Oct 6, 2021
@thbkrkr thbkrkr deleted the stop-reconciliation-on-es-license-reconciliation-error branch October 11, 2021 15:01
@david-kow david-kow changed the title Verify supported Elasticsearch distribution during licence reconciliation Verify supported Elasticsearch distribution during license reconciliation Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pebrc pebrc pebrc approved these changes

Assignees
No one assigned
Labels
>enhancement Enhancement of existing functionality v1.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thbkrkr @pebrc